To give some numbers for context here...

This reduces dynamic relocations by almost 4k on Linux, just over 4% of the non-vtable relocations. It's not as big as some of the previous reductions, but that's the nature of working down the stack...

The PR description says this wastes some space, so I specifically got bloaty comparison before and after this change, at least on my Linux system with a strip-ed binary from an optimized build:

    FILE SIZE        VM SIZE
 --------------  --------------
  +0.4%  +200Ki  +0.4%  +200Ki    .rodata
  [ = ]       0  +135% +1.03Ki    .relro_padding
  +0.0%      +4  +0.0%      +4    .eh_frame
 -12.5%      -4  [ = ]       0    [Unmapped]
  -5.0%     -40  -5.0%     -40    [LOAD #2 [R]]
  -1.4% -90.4Ki  -1.4% -90.4Ki    .rela.dyn
  -2.8%  -115Ki  -2.8%  -115Ki    .data.rel.ro
  -0.0% -5.03Ki  -0.0% -4.00Ki    TOTAL

No regression in overall size. The growth in .rodata is larger than the reduction in .data.rel.ro, reflecting exactly what the description suggests. But the additional reduction in .rela.dyn makes up for this. Ultimately, this is a wash for binary size, but more efficient for loading PIE binaries.

It might be nice to use a string table and an offset, but it'd barely be better given the small size of these strings.

@llvm/pr-subscribers-backend-risc-v
@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-backend-arm

Author: Owen Anderson (resistor)

diff --git a/llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h b/llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h
index 8f34cf054fe286..e0ccba4d6a59e8 100644
--- a/llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h
+++ b/llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h
@@ -700,8 +700,8 @@ AArch64StringToVectorLayout(StringRef LayoutStr) {
 
 namespace AArch64SysReg {
   struct SysReg {
-    const char *Name;
-    const char *AltName;
+    const char Name[32];
+    const char AltName[32];
     unsigned Encoding;
     bool Readable;
     bool Writeable;
diff --git a/llvm/lib/Target/ARM/Utils/ARMBaseInfo.h b/llvm/lib/Target/ARM/Utils/ARMBaseInfo.h
index 56a925f09ea7ae..5562572c5abf48 100644
--- a/llvm/lib/Target/ARM/Utils/ARMBaseInfo.h
+++ b/llvm/lib/Target/ARM/Utils/ARMBaseInfo.h
@@ -189,7 +189,7 @@ inline static unsigned ARMCondCodeFromString(StringRef CC) {
 // System Registers
 namespace ARMSysReg {
   struct MClassSysReg {
-    const char *Name;
+    const char Name[32];
     uint16_t M1Encoding12;
     uint16_t M2M3Encoding8;
     uint16_t Encoding;
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
index 9e36d62352ae51..fe28195654fdea 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
@@ -449,9 +449,9 @@ int getLoadFPImm(APFloat FPImm);
 
 namespace RISCVSysReg {
 struct SysReg {
-  const char *Name;
-  const char *AltName;
-  const char *DeprecatedName;
+  const char Name[32];
+  const char AltName[32];
+  const char DeprecatedName[32];
   unsigned Encoding;
   // FIXME: add these additional fields when needed.
   // Privilege Access: Read, Write, Read-Only.

How good is the error checking, if a future RISC-V extension adds a sysreg with a longer name? Do we need to find a place for a build-time error with a nice message?

How good is the error checking, if a future RISC-V extension adds a sysreg with a longer name? Do we need to find a place for a build-time error with a nice message?

The error checking seems fine to me:

/Users/resistor/llvm-project/build-release/lib/Target/RISCV/RISCVGenSearchableTables.inc:34559:5: error: initializer-string for char array is too long, array size is 32 but initializer has size 49 (including the null terminating character)
 34559 |   { "testtesttesttesttesttesttesttesttesttesttesttest", "testtesttesttesttesttesttesttesttesttesttesttest", "", 0x283,  {} , false }, // 64
       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Why not offsets into a big array of characters like we do for registers themselves? That would be more space efficient than padding all strings like this.

Why not offsets into a big array of characters like we do for registers themselves? That would be more space efficient than padding all strings like this.

I believe that is what @chandlerc was suggesting above:

It might be nice to use a string table and an offset, but it'd barely be better given the small size of these strings.

I haven't had a chance to look into it yet.

Why not offsets into a big array of characters like we do for registers themselves? That would be more space efficient than padding all strings like this.

That would need new support for that kind of "searchable table" in TableGen I think. Unless we already have it?

Why not offsets into a big array of characters like we do for registers themselves? That would be more space efficient than padding all strings like this.

That would need new support for that kind of "searchable table" in TableGen I think. Unless we already have it?

We do have a utility that automates building these pretty well: https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/TableGen/StringToOffsetTable.h

Maybe you mean connecting it differently to something else?

Why not offsets into a big array of characters like we do for registers themselves? That would be more space efficient than padding all strings like this.

That would need new support for that kind of "searchable table" in TableGen I think. Unless we already have it?

We do have a utility that automates building these pretty well: https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/TableGen/StringToOffsetTable.h

Maybe you mean connecting it differently to something else?

sysregs aren't a first class type in the tablegen sources. They are declared in the .td file as a GenericTable.

def SysRegsList : GenericTable {                                                 
  let FilterClass = "SysReg";                                                    
  // FIXME: add "ReadWrite", "Mode", "Extra", "Number" fields when needed.       
  let Fields = [                                                                 
    "Name", "AltName", "DeprecatedName", "Encoding", "FeaturesRequired",         
    "isRV32Only",                                                                
  ];                                                                             
                                                                                 
  let PrimaryKey = [ "Encoding" ];                                               
  let PrimaryKeyName = "lookupSysRegByEncoding";                                 
  let PrimaryKeyReturnRange = true;                                              
}  

I don't think the emitter that handles GenericTable knows how to create an array of characters.

sysregs aren't a first class type in the tablegen sources. They are declared in the .td file as a GenericTable.

...

I don't think the emitter that handles GenericTable knows how to create an array of characters.

Yeah, it'd need some custom emission to wire up everything. Some of this is why I had nudged Owen -- it wasn't at all clear to me how to effectively restructure the tablegen of the backends to more systematically use offsets into a string table.

Anyways...

I don't see a straightforward path to moving this to a string table. I did some prototyping of attempting to move GenericTable to use string tables for all string values, but it ends up being tricky for a few reasons:

• TableGen doesn't have a very strong type-system, so it's very hard to determine which things should be put in a string table.
• Some of the users of GenericTable don't have an easy way to recover the base pointer for the string table.
• Introducing SequenceToOffsetTable is mechanically difficult in the SearchableTableEmitter because it wants to be able to sort things lexicographically earlier than it would be natural to build the SequenceToOffsetTable.

This seems like it could be an area for future enhancement if someone wants to do some major refactoring, but it's not a simple add-on improvement.

Ping